Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Jun 24, 2025

User description

2025-06-24_03-20


PR Type

Enhancement


Description

  • Enhance API key error messaging for forks

  • Add fork detection via GITHUB_EVENT_PATH

  • Include docs link in error message

  • Import json and adjust type hints


Changes walkthrough 📝

Relevant files
Error handling
env_utils.py
Fork detection and improved API key errors                             

codeflash/code_utils/env_utils.py

  • Imported json for parsing GitHub event data
  • Added get_cached_gh_event_data to read GITHUB_EVENT_PATH
  • Implemented is_repo_a_fork to detect forked repositories
  • Enhanced get_codeflash_api_key error messages for forks
  • Included documentation link in API key errors
  • +28/-2   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing exception handling

    get_cached_gh_event_data opens and parses a file without catching FileNotFoundError or JSONDecodeError. A malformed or missing GITHUB_EVENT_PATH file could raise uncaught exceptions and crash the process.

    def get_cached_gh_event_data() -> dict[str, Any] | None:
        event_path = os.getenv("GITHUB_EVENT_PATH")
        if not event_path:
            return None
        with Path(event_path).open() as f:
            return json.load(f)  # type: ignore  # noqa
    Unused import

    The Optional type imported on line 8 is not used in this module and can be removed to clean up imports.

    from typing import Any, Optional

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent KeyError on fork check

    Avoid a potential KeyError by safely accessing the repository data. Use dict.get()
    with default values to handle missing or malformed keys.

    codeflash/code_utils/env_utils.py [98-102]

     @lru_cache(maxsize=1)
     def is_repo_a_fork() -> bool:
         event = get_cached_gh_event_data()
    -    if event is None:
    +    if not event:
             return False
    -    return bool(event["repository"]["fork"])
    +    return bool(event.get("repository", {}).get("fork", False))
    Suggestion importance[1-10]: 6

    __

    Why: Using event.get("repository", {}).get("fork", False) prevents a possible KeyError if the JSON payload lacks those keys, improving robustness without major impact.

    Low
    Check file existence before open

    Ensure the event file exists before opening to avoid a FileNotFoundError. Return
    None immediately if the path is missing or not a file.

    codeflash/code_utils/env_utils.py [106-111]

     @lru_cache(maxsize=1)
     def get_cached_gh_event_data() -> dict[str, Any] | None:
         event_path = os.getenv("GITHUB_EVENT_PATH")
    -    if not event_path:
    +    if not event_path or not Path(event_path).is_file():
             return None
         with Path(event_path).open() as f:
    -        return json.load(f)  # type: ignore  # noqa
    +        return json.load(f)
    Suggestion importance[1-10]: 6

    __

    Why: Verifying Path(event_path).is_file() avoids a FileNotFoundError when opening the event file, adding necessary defensive checks for reliability.

    Low

    codeflash-ai bot added a commit that referenced this pull request Jun 24, 2025
    …-on-missing-key-in-fork`)
    
    Here’s a **faster** version, making these improvements.
    
    - Avoid repeatedly calling `os.getenv()` and `Path.open()` for the same event file, by using an explicit cache variable for the file contents. This accelerates repeated lookups and reduces disk accesses.
    - Remove double @lru_cache use, as the value from `get_cached_gh_event_data()` will not change for a process lifetime and can be memory-cached explicitly.
    - Microoptimize the `is_repo_a_fork` logic by removing an unnecessary bool(...) coercion (the value is already True/False).
    
    Here’s the rewritten program.
    
    
    
    **Key notes:**
    - The event file is read at most once per process.
    - All redundant calls and mutable global state are avoided.
    - Fast subsequent access due to a module-level cache; you’ll see lower file-system and JSON load latency.
    - The returned values are identical for the same environment and state, and function signatures are maintained.
    
    Let me know if you want further micro-optimizations!
    @codeflash-ai
    Copy link
    Contributor

    codeflash-ai bot commented Jun 24, 2025

    ⚡️ Codeflash found optimizations for this PR

    📄 40% (0.40x) speedup for is_repo_a_fork in codeflash/code_utils/env_utils.py

    ⏱️ Runtime : 50.5 microseconds 36.0 microseconds (best of 91 runs)

    I created a new dependent PR with the suggested changes. Please review:

    If you approve, it will be merged into this PR (branch chore/error-on-missing-key-in-fork).

    codeflash-ai bot added a commit that referenced this pull request Jun 24, 2025
    …hore/error-on-missing-key-in-fork`)
    
    Here’s a faster and more memory-efficient version of your program.  
    **Optimization rationale:**
    - Avoids the extra `Path` object creation and `.open()` call—using `open(event_path)` is faster for a single use.
    - Reduces dependency usage by not involving `Path` unless file system path manipulation is needed.
    - Leaves error handling as originally absent, maintaining original logic.
    - Leaves `lru_cache` as is and all function/return types unchanged.
    
    
    **Summary of change:**  
    Direct file open instead of through `Path`, removing unnecessary abstraction for a single read and reducing overhead. All logic and return values are identical.
    KRRT7
    KRRT7 previously approved these changes Jun 24, 2025
    Comment on lines 45 to 51
    if is_repo_a_fork():
    msg = (
    "Codeflash API key not detected in your environment. It appears you're running Codeflash from a GitHub fork.\n"
    "For external contributors, please ensure you've added your own API key to your fork's repository secrets and set it as the CODEFLASH_API_KEY environment variable.\n"
    f"{api_secret_docs_message}"
    )
    raise OSError(msg)
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    use codeflash/code_utils/code_utils.py::exit_with_message

    do we want to exit with an error code since that will mark the GHA run as failed? @misrasaurabh1 thoughts there?

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @mohammedahmed18 meant to request changes here, not approve, make the changes please

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @mohammedahmed18 mohammedahmed18 requested a review from KRRT7 June 24, 2025 00:50
    @mohammedahmed18 mohammedahmed18 changed the title [Chore] Error message on missing API key for forks [Chore] Error message on missing API key for forks (CF-675) Jun 24, 2025
    @misrasaurabh1 misrasaurabh1 enabled auto-merge June 26, 2025 15:47
    @misrasaurabh1 misrasaurabh1 merged commit 2344c2c into main Jun 26, 2025
    16 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants